Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/9.0] [NativeAOT] Introduce pointer-based CompareExchange intrinsic and use operating with syncblock bits. #106727

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 20, 2024

Backport of #106703 to release/9.0

/cc @VSadov

Customer Impact

  • Customer reported
  • Found internally

When a NativeAOT app is compiled for Debug, use of monitor locks (as in lock(obj){...} statement), could in rare cases lock a wrong object or cause a crash in GC.

This was causing intermittent build failures in CI, about once a day, when the bug made it into toolset and ILC itself was impacted.

Regression

  • Yes
  • No

The root cause is pre-9.0, but was not exposed as a bug due to several mitigating factors.
Making CompareExchange intrinsic self-referential in #92974 (Implement Interlocked for small types) indirectly exposed the bug.

Since this is a stress bug with relatively rare occurrence, it took a while for the issue to propagate to the toolset compiler and then present itself in a form of CI build failures due to crashing ILC

Testing

Verified locally by examining the codegen for both x64 and arm64, and by running a directed repro in a loop 300 times.
(with directed repro without a fix a crash happens after 5-10 iterations)

There is no way to add a regression test for this right now. We mostly rely on CoreCLR for GC-stress, but this scenario is one of a few cases specific to NativeAOT and not reachable in CoreCLR stress tests.
We will be discussing how to improve our GC-stress situation with NativeAOT.

Risk

Low. The actual change is fairly small. We rely on existing JIT code that expands CompareExchange intrinsic for ref int, just introduced an internal entry point that takes int* as location - so that unsafe GC byref to an object syncblock will have no way to show up.
Regardless if CompareExchange intrinsic is expanded or not, the new entry point has no byrefs in it. Emitted code is the same as before, but without GC tracking of the location argument.

Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@VSadov VSadov force-pushed the backport/pr-106703-to-release/9.0 branch from 778bdaa to df4736a Compare August 21, 2024 15:31
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved. we can merge when ready

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 21, 2024
@jeffschwMSFT jeffschwMSFT merged commit 02bb8c3 into release/9.0 Aug 21, 2024
11 of 18 checks passed
@VSadov
Copy link
Member

VSadov commented Aug 21, 2024

Thanks!!

@VSadov VSadov deleted the backport/pr-106703-to-release/9.0 branch August 21, 2024 18:46
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants